perf: fix cpu usage in cache #626
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stas-nc
force-pushed
the
perf/fix-cpu-perf
branch
2 times, most recently
from
e6dc7fe to
cce3198
Compare
…s serialization/deserialization on each call to cache +ts refactoring
stas-nc
force-pushed
the
perf/fix-cpu-perf
branch
from
cce3198 to
277973e
Compare
stas-nc
changed the title
|
I've briefly reviewed this PR while it was in draft. Could u please elaborate on the reason of leakage? |
yehor-manzhula
approved these changes
Nov 14, 2024
|
Took flame chart during a load test to the local ILC server connected to the production registry (see attached file) in the jira ticket. |
wRLSS
reviewed
Nov 15, 2024
ilc/common/MemoryCacheStorage.ts
Outdated
| @@ -0,0 +1,12 @@ | |||
| import { CacheResult, CacheStorage } from './types/CacheWrapper'; | |||
|
|
|||
| export class MemoryCacheStorage implements CacheStorage { | |||
There was a problem hiding this comment.
Since we do not limit cache in any way it might lead to memory leak during long-running processes. I suggest that instead we create EvictingCacheStorage based on some sort of LRU algorithm
There was a problem hiding this comment.
@wRLSS Despite the fact that your comment is 100% valid, I'd suggest addressing it within the scope of another task because, as far as I can see, the approach to storing items in memory hasn't changed. This PR simply removes serialization/deserialization, so it won't make things any worse.
There was a problem hiding this comment.
I think the change is not that big and since we are fine-tuning ILC it can be done, small class update based on gpt suggestion without large impact. But I'm fine doing this in other PR, up to @stas-nc
There was a problem hiding this comment.
I can do it, but was not sure, because we have some kind of pre-heat logic, in this case, if the ilc server is idle we are losing it's benefit.
There was a problem hiding this comment.
I can do it, but was not sure, because we have some kind of pre-heat logic, in this case, if the ilc server is idle we are losing it's benefit.
It is possible to do evicting focused on number of usage rather than time so idleness won't affect it. Anyways, approving this PR, but I think we should check this case
wRLSS
approved these changes
Nov 15, 2024
blackrabbit99
approved these changes
Nov 15, 2024
yehor-manzhula
approved these changes
Nov 15, 2024
…r plugin Additionally fixed system js file, because banner plugin evaluates placeholders like [id] [name] etc.
Coverage ReportIlc/serverCommit SHA:465669aabbe64bba827c648a3e9af30f54554b85 Test coverage results 🧪File details
Ilc/clientCommit SHA:465669aabbe64bba827c648a3e9af30f54554b85 Test coverage results 🧪File details
RegistryCommit SHA:465669aabbe64bba827c648a3e9af30f54554b85 Test coverage results 🧪File details
|
b1ff
approved these changes
Nov 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.